ci: optimize pipeline for 2-3x speed improvement #762
Conversation
📝 WalkthroughWalkthroughThis PR removes separate CI workflows for format, lint, unit, and integration and adds a consolidated PR CI workflow that builds in a Rust container, uploads build artifacts, and runs matrixed integration tests. The setup-build-env action is simplified (system deps installed without sudo), cache behavior updated to use rust-cache with ~/.cargo and ~/.rustup, and setup-solana adds caching plus conditional install. Tests now run with pre-built validator binaries and cargo test uses --locked. Workspace version bumped to 0.5.1 and Clippy invocations use --no-deps. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/actions/setup-solana/action.yml (1)
7-18: Consider adding cache validation to prevent issues with corrupted caches.The cache key is static (version-based), and the installation only checks for binary existence. If the cache becomes corrupted but the binary file exists, the installation step will be skipped, potentially causing runtime failures.
Consider adding a lightweight validation step (e.g.,
solana-test-validator --version) after the cache restore to verify integrity before skipping installation.🔎 Proposed validation check
- name: Install Solana Test Validator shell: "bash" run: | - if [ ! -f ~/.local/share/solana/install/active_release/bin/solana-test-validator ]; then + if [ ! -f ~/.local/share/solana/install/active_release/bin/solana-test-validator ] || ! ~/.local/share/solana/install/active_release/bin/solana-test-validator --version; then sh -c "$(curl -sSfL https://release.anza.xyz/v2.2.20/install)" fi echo "$HOME/.local/share/solana/install/active_release/bin" >> $GITHUB_PATHtest-integration/test-tools/src/validator.rs (1)
36-57: Consider adding an explicit check for binary existence with a helpful error message.The validator now executes a pre-built binary, but there's no check to verify it exists before attempting to spawn. If the binary is missing (e.g., first CI run, local development without
cargo build), the current error message "Failed to start validator" won't clearly indicate the root cause.Consider adding an existence check with a more informative error:
🔎 Proposed improved error handling
// Start validator directly from the pre-built binary // (already compiled in CI build step or by local `cargo build`) let validator_bin = root_dir.join("target/debug/magicblock-validator"); + if !validator_bin.exists() { + panic!( + "Pre-built validator binary not found at {:?}. Run `cargo build` first.", + validator_bin + ); + } let mut command = process::Command::new(&validator_bin); let keypair_base58 = loaded_chain_accounts.validator_authority_base58(); let rust_log_style = std::env::var("RUST_LOG_STYLE").unwrap_or(log_suffix.to_string()); command .arg(config_path) .env("RUST_LOG_STYLE", rust_log_style) .env("MBV_VALIDATOR__KEYPAIR", keypair_base58.clone()) .current_dir(root_dir); eprintln!("Starting validator with {:?}", command); eprintln!( "Setting validator keypair to {} ({})", loaded_chain_accounts.validator_authority(), keypair_base58 ); let validator = command.spawn().expect("Failed to start validator");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/actions/setup-build-env/action.yml(2 hunks).github/actions/setup-solana/action.yml(1 hunks).github/workflows/ci-fmt.yml(1 hunks).github/workflows/ci-lint.yml(1 hunks).github/workflows/ci-test-integration.yml(4 hunks).github/workflows/ci-test-unit.yml(1 hunks).github/workflows/publish-packages.yml(1 hunks)test-integration/test-runner/bin/run_tests.rs(1 hunks)test-integration/test-tools/src/validator.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
.github/workflows/ci-lint.yml.github/workflows/ci-test-unit.yml.github/workflows/ci-fmt.yml.github/workflows/ci-test-integration.yml
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
test-integration/test-tools/src/validator.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/test-tools/src/validator.rs.github/workflows/ci-test-integration.yml
🪛 actionlint (1.7.9)
.github/workflows/ci-test-integration.yml
51-51: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
92-92: the runner of "actions/download-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_format
- GitHub Check: run_make_ci_lint
🔇 Additional comments (11)
.github/workflows/publish-packages.yml (1)
67-67: LGTM! Toolchain version aligned with other workflows.The Rust toolchain version update to 1.91.1 aligns with the unified toolchain strategy across CI workflows, ensuring consistency.
.github/workflows/ci-test-unit.yml (1)
25-25: LGTM! Dynamic cache key improves invalidation strategy.The hash-based cache key using Cargo.lock files ensures automatic cache invalidation when dependencies change, while the
-v001suffix allows manual cache busting when needed. This aligns with the PR's objective of optimizing cache usage.test-integration/test-runner/bin/run_tests.rs (1)
786-787: LGTM! --locked flag ensures reproducible test execution.Adding
--lockedto the cargo test command enforces the use of exact dependency versions fromCargo.lock, preventing unexpected dependency resolution during test runs. This improves reproducibility and aligns with CI best practices..github/workflows/ci-lint.yml (1)
25-25: LGTM! Cache key properly unified for cross-workflow sharing.The cache key format matches
ci-test-unit.yml(without workflow-specific prefixes), enabling cache sharing across workflows as intended by the PR objectives..github/actions/setup-build-env/action.yml (2)
27-29: LGTM! dtolnay/rust-toolchain is the recommended approach.Switching to the
dtolnay/rust-toolchainaction is the community-recommended standard for Rust toolchain management in GitHub Actions, providing better caching and reliability.
45-45: Cache path change is correct. The test-integration directory is confirmed to be a separate Cargo workspace with its ownCargo.toml. Since no target-dir overrides exist, Cargo builds this workspace totest-integration/targetby default. The updated cache pathmagicblock-validator/test-integration -> test-integration/targetcorrectly reflects this build output location..github/workflows/ci-test-integration.yml (5)
26-26: LGTM! Unified cache key strategy is well-designed.The cache key now incorporates hashes of both
Cargo.lockfiles, ensuring automatic invalidation when dependencies change. Using the same key across both jobs enables efficient artifact reuse, directly supporting the PR's performance goals.Also applies to: 84-84
40-48: LGTM! Pre-compilation strategy aligns with optimization goals.The separate build steps for the test-runner binary and test binaries enable reuse across integration test jobs, eliminating redundant compilation. The
--lockedflag ensures reproducible builds.
97-99: LGTM! Permission restoration is necessary.The
chmod +xstep correctly restores execute permissions lost during artifact upload/download.
111-111: LGTM! Environment variable correctly wires the pre-built binary.The
TEST_RUNNER_BINvariable passes the absolute path to the pre-built test-runner binary, enabling the integration tests to bypass redundant compilation.
85-85: LGTM! Rust version aligns with unification strategy.The explicit Rust version (1.91.1) matches the PR's objective of standardizing toolchain versions across workflows.
bmuddha
force-pushed
the
bmuddha/ci/it-speed-up
branch
from
aefa524 to
74778fb
Compare
bmuddha
requested review from
GabrielePicco,
lucacillario,
taco-paco and
thlorenz
as code owners
bmuddha
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci-fmt.yml (1)
24-24: Cache key prefix inconsistency prevents unified caching.This issue was already raised in a previous review: the
ci-fmt-prefix creates a separate cache for this workflow, which contradicts the PR objective of unified cache keys across all workflows. Removing this prefix would enable cache sharing withci-test-unit.ymlandci-lint.yml.
🧹 Nitpick comments (1)
.github/actions/setup-build-env/action.yml (1)
27-30: Pin rust-toolchain action to a specific version.Using
@masterfor thedtolnay/rust-toolchainaction can introduce unexpected breaking changes. Pin to a specific release tag (e.g.,@1.xor@stable) to ensure reproducible builds.🔎 Proposed fix for version pinning
- uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@1.0
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/actions/setup-build-env/action.yml(2 hunks).github/actions/setup-solana/action.yml(1 hunks).github/workflows/ci-fmt.yml(1 hunks).github/workflows/ci-lint.yml(1 hunks).github/workflows/ci-test-integration.yml(4 hunks).github/workflows/ci-test-unit.yml(1 hunks).github/workflows/publish-packages.yml(1 hunks)test-integration/test-runner/bin/run_tests.rs(1 hunks)test-integration/test-tools/src/validator.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/ci-lint.yml
- .github/workflows/publish-packages.yml
- .github/actions/setup-solana/action.yml
- test-integration/test-tools/src/validator.rs
- test-integration/test-runner/bin/run_tests.rs
- .github/workflows/ci-test-integration.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
.github/workflows/ci-test-unit.yml.github/workflows/ci-fmt.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
🔇 Additional comments (1)
.github/workflows/ci-test-unit.yml (1)
25-25: LGTM! Dynamic cache key aligns with PR objectives.The change from a static cache key to a dynamic hash-based key using
hashFilescorrectly implements the unified caching strategy described in the PR objectives. This will enable cache sharing across workflows while maintaining cache validity based on dependency changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/checks.yml (3)
22-23: Upgradeactions/checkoutfrom v2 to v4.
actions/checkout@v2is deprecated and may have compatibility issues with newer runner environments. This affects lines 23, 59, 82, 122, and 157.🔎 Proposed fix
Update all checkout steps:
- name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4
100-102: Consider parallelizing integration tests with unit tests.Integration tests currently depend on
nextest, creating a sequential pipeline: build → nextest → integration tests. If integration tests only need the build artifacts, changingneeds: buildwould allow unit and integration tests to run in parallel, potentially reducing total CI time.However, if the intent is to gate expensive integration tests behind cheaper unit tests, the current sequential approach is reasonable.
159-163: Consider pinningdtolnay/rust-toolchainto a specific version.Using
@masterfor the action means the workflow could break if the action introduces breaking changes. Consider pinning to a tagged version (e.g.,@1.86.0or a commit SHA) for reproducibility.🔎 Example
- name: Install Rust - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@stable with: toolchain: nightly components: rustfmtNote: The
@stabletag or a commit SHA provides more stability than@master.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/checks.yml(1 hunks).github/workflows/ci-fmt.yml(0 hunks).github/workflows/ci-lint.yml(0 hunks).github/workflows/ci-test-integration.yml(0 hunks).github/workflows/ci-test-unit.yml(0 hunks)
💤 Files with no reviewable changes (4)
- .github/workflows/ci-fmt.yml
- .github/workflows/ci-test-unit.yml
- .github/workflows/ci-test-integration.yml
- .github/workflows/ci-lint.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
🪛 actionlint (1.7.9)
.github/workflows/checks.yml
14-14: calling function "hashFiles" is not allowed here. "hashFiles" is only available in "jobs.<job_id>.steps.continue-on-error", "jobs.<job_id>.steps.env", "jobs.<job_id>.steps.if", "jobs.<job_id>.steps.name", "jobs.<job_id>.steps.run", "jobs.<job_id>.steps.timeout-minutes", "jobs.<job_id>.steps.with", "jobs.<job_id>.steps.working-directory". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
19-19: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
23-23: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
59-59: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
78-78: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
82-82: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
103-103: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
122-122: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
157-157: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/checks.yml (2)
53-74: Clippy job structure looks good.Correctly depends on
buildjob to leverage the cache, and appropriately uses a standard runner for linting which is less resource-intensive than builds.
76-98: Unit test job configuration is well-structured.Resource limit adjustments for file descriptors and inotify are appropriate for test workloads. The dependency on the build job ensures the cache is populated before running tests.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/README.md (1)
17-28: Consider adding language specifiers to fenced code blocks.For better rendering and accessibility, specify the language for fenced code blocks.
🔎 Proposed fix
-``` +```text build (ubuntu-latest-m) ├── clippy (depends on build) → runs on ubuntu-latest └── nextest (depends on build) → runs on ubuntu-latest-m fmt (independent, ubuntu-latest) → runs in parallel with buildCache key:
-+yaml
magicblock-validator-${HASH(Cargo.lock, test-integration/Cargo.lock)}-build-v001.github/workflows/checks.yml (1)
15-15: Consider configuring actionlint for custom runners.If
ubuntu-latest-mis a self-hosted or custom runner label, add it to youractionlint.yamlconfiguration to suppress linting warnings:# .github/actionlint.yaml self-hosted-runner: labels: - ubuntu-latest-m
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/README.md(1 hunks).github/workflows/checks.yml(1 hunks)Makefile(1 hunks)test-integration/Makefile(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
🪛 actionlint (1.7.9)
.github/workflows/checks.yml
15-15: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
60-60: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
81-81: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
85-85: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
108-108: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
127-127: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
164-164: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 markdownlint-cli2 (0.18.1)
.github/workflows/README.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (6)
test-integration/Makefile (1)
217-217: LGTM! Appropriate clippy optimization.Adding
--no-depsfocuses linting on the project's own code, excluding dependencies. This is a sensible optimization that reduces CI time while maintaining code quality checks where they matter.Makefile (1)
49-49: LGTM! Consistent optimization with test-integration.The
--no-depsflag mirrors the change intest-integration/Makefile, maintaining consistency across the project's lint configuration..github/workflows/README.md (1)
1-74: Documentation is clear and helpful.The README effectively documents the CI optimization strategy and migration path. Well-structured and informative.
.github/workflows/checks.yml (3)
54-78: LGTM! Clippy job properly structured.The job correctly:
- Depends on the build job to ensure cache warmth
- Reuses the unified cache key
- Runs clippy in both the main workspace and test-integration
80-104: LGTM! Nextest job well-configured.The job correctly:
- Depends on the build job
- Adjusts system limits for test execution
- Reuses the unified cache
160-178: LGTM! Format check job properly configured.The fmt job correctly:
- Runs independently without cache overhead
- Uses nightly Rust with rustfmt component
- Checks formatting in both workspaces
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/checks.yml (2)
19-19: Update to actions/checkout@v4.The
actions/checkout@v2action is deprecated and no longer supported on GitHub Actions. Update tov4for continued support and security patches. This issue was flagged in previous reviews but remains unaddressed.🔎 Proposed fix for all checkout actions
Apply this change to lines 19, 81, and 109:
- name: Checkout - uses: actions/checkout@v2 + uses: actions/checkout@v4
103-103: The TEST_RUNNER_BIN environment variable is set but never used.Previous reviews confirmed that
test-integration/Makefile'stesttarget invokescargo run --package test-runner --bin run-tests, which rebuilds the binary instead of using the pre-built binary path set inTEST_RUNNER_BIN. This defeats the optimization goal. Update the Makefile to execute the pre-built binary directly.🔎 Recommended fix
Modify
test-integration/Makefileto use the pre-built binary:test: - cargo run --package test-runner --bin run-tests -- $(RUN_TESTS) + $(TEST_RUNNER_BIN) $(RUN_TESTS)Ensure
TEST_RUNNER_BINis set to the correct path before calling the test target.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/actions/setup-build-env/action.yml(2 hunks).github/workflows/checks.yml(1 hunks)Cargo.toml(1 hunks)Makefile(1 hunks)test-integration/Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- test-integration/Makefile
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
.github/workflows/checks.yml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
.github/workflows/checks.yml
🪛 actionlint (1.7.9)
.github/workflows/checks.yml
15-15: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
62-62: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
81-81: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
109-109: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Lint & Test
🔇 Additional comments (4)
.github/actions/setup-build-env/action.yml (2)
26-32: LGTM! Effective Rust toolchain caching.The caching configuration correctly targets
~/.rustup/toolchainswith an appropriate key strategy that varies by toolchain version and OS, enabling efficient reuse across workflow runs.
33-37: LGTM! Proper use of dtolnay/rust-toolchain.Switching to the dtolnay/rust-toolchain action is an idiomatic improvement. The
@masterreference is correct for this action (it doesn't publish versioned releases), and the specified components (rustfmt, clippy) align with the workflows' requirements.Makefile (1)
49-49: LGTM: Clippy scope optimization aligns with CI speedup goals.Switching from
--all-targetsto--no-depsnarrows the linting scope to exclude dependencies, reducing analysis time while maintaining correctness checks on project code..github/workflows/checks.yml (1)
15-15: Remove the runner label concern —ubuntu-latest-mis a valid GitHub Actions larger runner label.
ubuntu-latest-mis an officially supported GitHub Actions runner label for medium-sized larger runners (4 CPU cores, 16 GB RAM, ~150 GB disk), available to organizations with the GitHub Actions larger runners feature enabled. No action needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/checks.yml (3)
19-19: Update to actions/checkout@v4.The
actions/checkout@v2action is deprecated and may stop working. Update tov4for continued support and security updates.🔎 Proposed fix for all checkout actions
Apply this change to lines 19, 81, and 112:
- - name: Checkout - uses: actions/checkout@v2 + - name: Checkout + uses: actions/checkout@v4
33-43: Verify that built artifacts are available via cache in integration test jobs.The PR objectives state "Pre-compiled binaries: test-runner and test binaries compiled once and reused," but this workflow relies on Rust cache (
Swatinem/rust-cache) rather than explicit artifact upload/download to share builds between jobs. While the shared cache key should theoretically preserve artifacts, this approach is less reliable than explicit artifact passing:
- Cache restoration is not guaranteed across jobs/runners
- The verification step at line 96 ("Verify magicblock-validator binary exists") suggests uncertainty about cache reliability
- If cache misses or is stale, integration tests will rebuild everything, defeating the optimization
Run the following script to check if integration tests actually use pre-built binaries or rebuild:
#!/bin/bash # Description: Check if ci-test-integration rebuilds binaries or uses cached ones echo "=== Checking ci-test-integration target in root Makefile ===" rg -nA 10 '^ci-test-integration:' Makefile echo -e "\n=== Checking test-integration/Makefile for cargo build/run commands ===" rg -n 'cargo (build|run)' test-integration/MakefileIf integration tests invoke
cargo buildorcargo run, they will rebuild binaries instead of reusing cached artifacts.
98-107: Verify that TEST_RUNNER_BIN is actually used by the Makefile.The
TEST_RUNNER_BINenvironment variable points to a pre-built test-runner binary, but according to previous reviews, theci-test-integrationMakefile target may invokecargo run --package test-runnerinstead, which rebuilds the binary and defeats the optimization.Run the following script to verify whether the Makefile uses the pre-built binary:
#!/bin/bash # Description: Check if test-integration Makefile uses TEST_RUNNER_BIN or rebuilds echo "=== Checking for TEST_RUNNER_BIN usage in test-integration/Makefile ===" rg -n 'TEST_RUNNER_BIN' test-integration/Makefile || echo "TEST_RUNNER_BIN not found in Makefile" echo -e "\n=== Checking test target in test-integration/Makefile ===" rg -nA 10 '^(test|ci-test):' test-integration/MakefileIf
TEST_RUNNER_BINis not referenced, update the Makefile to use$(TEST_RUNNER_BIN)instead ofcargo run.
🧹 Nitpick comments (1)
.github/workflows/checks.yml (1)
15-15: Consider configuring actionlint.yaml for custom runner labels.The
ubuntu-latest-mrunner label is flagged by actionlint as unknown. If this is a self-hosted runner, you can suppress this warning by adding the label to youractionlint.yamlconfiguration.Based on static analysis hints from actionlint.
Also applies to: 62-62
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-build-env/action.yml(3 hunks).github/workflows/checks.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
.github/workflows/checks.yml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
.github/workflows/checks.yml
🪛 actionlint (1.7.9)
.github/workflows/checks.yml
15-15: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
19-19: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
62-62: label "ubuntu-latest-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
81-81: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
112-112: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (3)
.github/actions/setup-build-env/action.yml (3)
17-20: LGTM!The new
install_system_depsinput adds useful flexibility for skipping system dependency installation when not needed.
30-43: LGTM!The Rust toolchain caching and migration to
dtolnay/rust-toolchainare excellent optimizations:
- Caching
~/.rustupand~/.cargoreduces toolchain installation time.- Using
dtolnay/rust-toolchainis the recommended approach and handles component installation cleanly.
57-59: LGTM!The workspace cache mappings are now correct:
. -> targetfor the root workspacetest-integration -> targetfor the separate test-integration workspaceThis properly addresses the issues flagged in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/checks.yml (2)
1-6: File comment has incorrect filename.Line 1 references
.github/workflows/ci.ymlbut the actual filename ischecks.yml. Update the comment to match the filename.🔎 Proposed fix
-# .github/workflows/ci.yml +# .github/workflows/checks.yml name: Run CI - Test & Lint
66-72:chmod +x *may fail or produce warnings on non-executable files.The
test-integration/target/debug/directory likely contains non-binary files (e.g.,.ddependency files,.fingerprintdirectories, incremental compilation artifacts). Usingchmod +x *on these is harmless but produces noise. Consider targeting only actual binaries.🔎 Proposed fix using find to target only files
- run: | mkdir -p target/debug mkdir -p test-integration/target/debug mv downloaded-artifacts/target/debug/magicblock-validator target/debug/ cp -r downloaded-artifacts/test-integration/target/debug/* test-integration/target/debug/ chmod +x target/debug/magicblock-validator - chmod +x test-integration/target/debug/* + find test-integration/target/debug -maxdepth 1 -type f -executable -exec chmod +x {} + 2>/dev/null || trueOr simply ignore errors since the binaries that matter will already be executable:
- chmod +x test-integration/target/debug/* + chmod +x test-integration/target/debug/* 2>/dev/null || true.github/actions/setup-build-env/action.yml (1)
31-44:save-if: alwaysis misleading; usetrueinstead.The
save-ifinput expects a boolean expression. Whilealwaysworks as a truthy string, it's not a recognized keyword here (unlike inif:conditions wherealways()is a function). For clarity, usetrueto explicitly indicate the cache should always be saved.🔎 Proposed fix
cache-targets: true cache-all-crates: true cache-on-failure: true - save-if: always + save-if: true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-build-env/action.yml(1 hunks).github/workflows/checks.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
.github/workflows/checks.yml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
.github/workflows/checks.yml
🔇 Additional comments (3)
.github/workflows/checks.yml (1)
14-42: Build job structure looks good.The build job correctly:
- Uses
actions/checkout@v4(addressing prior feedback)- Builds both workspaces and uploads artifacts
- Uses appropriate retention period
.github/actions/setup-build-env/action.yml (2)
21-29: System dependencies installation is container-appropriate.The comment correctly notes the root/container assumption. The package list covers the typical build requirements (libudev-dev for USB, libclang-dev for bindgen, build-essential for compilation).
35-40: Workspace paths and cache directories are correctly configured.The workspace mappings (
. -> targetandtest-integration -> target) properly target both workspaces' build outputs. This addresses the path issues flagged in prior reviews.
| # 4. Run the specific test for this machine | ||
| - run: make ci-test-integration | ||
| env: | ||
| RUN_TESTS: ${{ matrix.batch_tests }} | ||
| TEST_RUNNER_BIN: ${{ github.workspace }}/test-integration/target/debug/run-tests |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if TEST_RUNNER_BIN is used in test-integration/Makefile
echo "=== Searching for TEST_RUNNER_BIN usage in Makefiles ==="
rg -n 'TEST_RUNNER_BIN' Makefile test-integration/Makefile 2>/dev/null || echo "No TEST_RUNNER_BIN references found"
echo ""
echo "=== Checking ci-test-integration target ==="
rg -nA 10 '^ci-test-integration' Makefile 2>/dev/null || echo "Target not found in root Makefile"
echo ""
echo "=== Checking test target in test-integration/Makefile ==="
rg -nA 10 '^test:' test-integration/Makefile 2>/dev/null || echo "Target not found"
echo ""
echo "=== Looking for cargo run test-runner invocations ==="
rg -n 'cargo run.*test-runner' test-integration/Makefile Makefile 2>/dev/null || echo "No cargo run test-runner found"Repository: magicblock-labs/magicblock-validator
Length of output: 1374
The ci-test-integration target does not use the pre-built TEST_RUNNER_BIN; fix the Makefile to use it.
The test target in test-integration/Makefile (line 36) executes cargo run --package test-runner --bin run-tests instead of using the pre-built binary. This causes the test runner to rebuild on every test run, negating the optimization of precompiling the binary. The workflow passes TEST_RUNNER_BIN but the Makefile ignores it. Update the test target to use $(TEST_RUNNER_BIN) instead of rebuilding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/checks.yml (1)
1-1: Comment references wrong filename.The inline comment says
ci.ymlbut the file ischecks.yml. Consider updating for consistency.-# .github/workflows/ci.yml +# .github/workflows/checks.yml
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/checks.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
.github/workflows/checks.yml
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
.github/workflows/checks.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
.github/workflows/checks.yml (5)
4-10: Concurrency and trigger setup looks good.The workflow-level concurrency with
cancel-in-progress: trueefficiently cancels redundant runs on force-pushes. The PR event types cover all relevant scenarios.
44-54: Matrix parallelization strategy is well-designed.Good use of
fail-fast: falseto ensure all test batches complete and provide comprehensive feedback. The 11-way parallelization with prebuilt binaries aligns with the stated 2-3× speedup objectives.
66-72: Artifact restoration logic is correct.The path mapping correctly restores binaries to their expected locations. Minor note: the glob
test-integration/target/debug/*won't recursively chmod files in subdirectories (e.g.,deps/). If any needed executables are nested, consider usingfind:find test-integration/target/debug -type f -executable -exec chmod +x {} \;However, if all required binaries are at the top level, the current approach is fine.
74-85: Test execution setup looks correct.The
skip-restorecache key optimization avoids redundant Rust cache downloads since prebuilt binaries are already available. TheTEST_RUNNER_BINenvironment variable correctly points to the downloaded test runner.
36-42: The artifact upload totest-integration/target/debug/is ineffective and doesn't prevent rebuilds.The workflow downloads and copies the artifact but then runs
cargo run, which rebuilds the binary anyway. TheTEST_RUNNER_BINenvironment variable is set but never used by the Makefile or test-runner code, and the rust-cache step is configured toskip-restoredespite claiming "we have binaries!" Being more selective with artifact contents won't help—the real issue is that cargo lacks the full build metadata needed to reuse pre-built binaries, so they're reconstructed regardless.Either:
- Modify the test target to use
$TEST_RUNNER_BINwhen available (pre-built), falling back tocargo runif not set, or- Remove the ineffective artifact upload entirely and let the test machines rebuild locally
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/checks.yml (1)
68-74: Consider more precisechmodapplication.Line 74 applies execute permissions to all files in
test-integration/target/debug/*, including non-executable files like.ddependency files and intermediate build artifacts. While harmless, this is overly broad.🔎 More precise approach (optional)
If the exact binary names are known, target them specifically:
- run: | mkdir -p target/debug mkdir -p test-integration/target/debug mv downloaded-artifacts/target/debug/magicblock-validator target/debug/ cp -r downloaded-artifacts/test-integration/target/debug/* test-integration/target/debug/ chmod +x target/debug/magicblock-validator - chmod +x test-integration/target/debug/* + chmod +x test-integration/target/debug/run-tests + # Add other known test binaries if neededAlternatively, target only ELF binaries:
find test-integration/target/debug -maxdepth 1 -type f -executable -exec chmod +x {} \;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/checks.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
.github/workflows/checks.yml
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
.github/workflows/checks.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
.github/workflows/checks.yml (6)
1-11: LGTM! Workflow metadata and triggers are properly configured.The concurrency control will prevent wasted CI runs when pushing multiple commits in quick succession, and the trigger configuration covers all relevant PR events while correctly excluding drafts at the job level.
19-31: LGTM! Build environment setup is correct.The checkout action is up-to-date (v4), git safe directory configuration is appropriate for container execution, and the use of
"shared-cache-key"for the build cache aligns with the PR's objective of unifying cache keys across workflows for better cache reuse.
32-36: LGTM! Compilation steps are properly ordered.The build sequence correctly uses
--lockedfor reproducible builds and follows the logical order: root workspace → test programs → test-integration workspace. This ensures all dependencies are built before dependent artifacts.
38-45: LGTM! Artifact upload correctly implements the optimization strategy.The upload includes the validator binary and all test-integration artifacts, enabling the PR's core optimization: compile once in the build job, then reuse across multiple test matrix jobs. The 1-day retention is appropriate for ephemeral PR artifacts.
47-56: LGTM! Integration test job configuration optimizes for parallelism and visibility.The job correctly depends on
build, usesfail-fast: falseto run all test batches even when one fails (improving CI visibility), and properly justifies the--privilegedrequirement for kernel operations. The 11-way matrix parallelization aligns with the PR's performance optimization goals.
84-87: LGTM! Test execution correctly uses pre-built binaries.The
ci-test-integrationtarget now receivesTEST_RUNNER_BINpointing to the pre-downloaded binary, andRUN_TESTSselects the appropriate test batch from the matrix. This implements the PR's core optimization of avoiding redundant compilation.
| build: | ||
| if: github.event.pull_request.draft == false | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: rust:1.91.1-bookworm | ||
| options: --privileged |
There was a problem hiding this comment.
Remove unnecessary --privileged flag from the build job.
The build job only compiles code and uploads artifacts—it doesn't perform kernel-level operations like sysctl or prlimit that would require elevated container privileges. The --privileged flag unnecessarily expands the attack surface and should be removed to follow the principle of least privilege.
The integration_tests job correctly uses --privileged with a justifying comment (line 52), but the build job has no such requirement.
🔎 Proposed fix
build:
if: github.event.pull_request.draft == false
runs-on: ubuntu-latest
container:
image: rust:1.91.1-bookworm
- options: --privileged📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| build: | |
| if: github.event.pull_request.draft == false | |
| runs-on: ubuntu-latest | |
| container: | |
| image: rust:1.91.1-bookworm | |
| options: --privileged | |
| build: | |
| if: github.event.pull_request.draft == false | |
| runs-on: ubuntu-latest | |
| container: | |
| image: rust:1.91.1-bookworm |
| # 3. Setup Env (Only system deps needed, no heavy rust compile) | ||
| - uses: ./.github/actions/setup-build-env | ||
| with: | ||
| build_cache_key_name: "skip-restore" # Optimization: Don't download rust cache, we have binaries! | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| - uses: ./.github/actions/setup-solana |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/actions/setup-build-env/action.ymlRepository: magicblock-labs/magicblock-validator
Length of output: 1148
🏁 Script executed:
find .github/actions/setup-build-env -type f -name "*.sh" -o -name "*.js" -o -name "*.ts" | head -20Repository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
rg -n 'skip-restore' .github/actions/setup-build-env/ -A 3 -B 3Repository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
rg -n 'Swatinem/rust-cache' .github/workflows/ -A 5 -B 5Repository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
rg -n '"skip-restore"' . --type yaml --type jsonRepository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
cat -n .github/workflows/checks.yml | sed -n '70,90p'Repository: magicblock-labs/magicblock-validator
Length of output: 1118
🏁 Script executed:
rg -i 'skip-restore|skip.restore' . -A 2 -B 2Repository: magicblock-labs/magicblock-validator
Length of output: 62
🏁 Script executed:
git log --oneline -n 20 -- .github/workflows/checks.ymlRepository: magicblock-labs/magicblock-validator
Length of output: 111
🌐 Web query:
Swatinem/rust-cache action skip-restore parameter
💡 Result:
There is no parameter named skip-restore. Use lookup-only instead — when lookup-only: "true" the action checks the cache key and sets cache-hit but does not restore the cache. [1]
Source:
- Swatinem/rust-cache README (inputs section: lookup-only). [1]
The "skip-restore" optimization is not functional and requires correction.
The build_cache_key_name: "skip-restore" value is passed directly to Swatinem/rust-cache@v2 as the shared-key parameter, where it is treated as a literal cache key name, not a special directive. The rust-cache action does not recognize "skip-restore" as a signal to skip cache restoration. To actually skip cache restoration, use the lookup-only: true parameter instead. Either modify setup-build-env to handle this magic string and pass lookup-only: true to rust-cache, or update the workflow to pass lookup-only directly.
Summary
Implement comprehensive cargo cache reuse and toolchain caching to reduce CI execution time by 60-93 minutes (total parallel compute time) (65-70% reduction).
Changes
Results
Compatibility
Testing
Checklist
Summary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.